Skip to content

Fix/performance tab gaps - #91

Open
notSumit25 wants to merge 2 commits into
mainfrom
fix/performance-tab-gaps
Open

Fix/performance tab gaps#91
notSumit25 wants to merge 2 commits into
mainfrom
fix/performance-tab-gaps

Conversation

@notSumit25

Copy link
Copy Markdown
Collaborator

No description provided.

notSumit25 and others added 2 commits August 29, 2026 17:18
… review

The seven non-blocking findings left over after #86. Each was re-verified against
current main before being fixed, and each fix verified after — two of them turned
out to be wrong on the first attempt, both caught by testing rather than reading.

Sample recovery was an unindexable full scan on a table nothing pruned

recoverFullText runs one lookup per sample, up to 20 per "view full query" click,
and the query wrapped query_text in three nested regexp_replace/REPLACE calls plus
LOWER — so no index could satisfy it and Postgres materialized a rewritten copy of
the whole per-connection slice. EXPLAIN (ANALYZE) on a real install: 36 ms at 1,093
rows, 1,112 ms at 34,976. Linear, and query_lineage was absent from
SlowQueryRetentionService, so it degraded with the install's *age* rather than its
load — which is why it passed every pre-launch test.

Precomputed into a STORED generated column (normalized_match) with a
text_pattern_ops index, applied by QueryLineageMatchIndexInitializer since this repo
has no Flyway runtime. Verified on the real table: Index Scan, 0.119 ms, and the DDL
is idempotent on re-run. query_lineage is now purged with the other three fact
tables.

Deliberately no COALESCE fallback to the inline expression. That is the obvious way
to stay safe on a database without the column and it silently undoes the entire fix:
measured on the same 37,504-row table, COALESCE(normalized_match, …) plans a Seq Scan
at 48.7 ms versus 0.39 ms for the bare column. A missing column instead surfaces as a
WARN from the initializer, and recoverFullText already catches a failed lookup and
returns the sample unchanged.

Denials were retried three times

queryClient set retry: 3 with no status predicate, so a 403 became four requests and
~7 s of backoff before the UI could render anything — and an unauthorized
/tenant-column-suggestions opens a fresh JDBC connection to the target database on
every attempt.

The first version of this fix read error.response.status and did nothing at all: the
axios response interceptor rethrows a plain Error with the status copied onto
error.status, so the axios-shaped field never matched. Measured in the browser before
and after: 4 attempts / 7197 ms -> 1 attempt / 34 ms. Confirmed 500, 503, 401 and
network failures still retry, and the 3-attempt cap still holds.

A customer id containing a slash was unreachable by any encoding

customerId is a literal value from the tenant column — application data, so it can
contain /, ? or #. Raw, the slash split the path; percent-encoded, Jetty answers 400
"Ambiguous URI path separator". Both reproduced with the real value `acct/77?x=1`,
whose rows rendered as "no queries rolled up yet" while the header said the customer
had 12 executions. encodeURIComponent alone does not fix this, which is why the id
moved off the path: /{connectionId}/customer-queries?customerId=… and
/customer-query-samples. The old path routes are kept and @deprecated for wire
compatibility. Verified: the slash-bearing id now returns 200, and 403 on a
connection the caller cannot read.

Failed fetches rendered as "no data yet"

Every panel branched on `!isLoading && rows.length === 0`, and `data ?? []` turns any
error into an empty array — so a 404 and an empty result were indistinguishable. That
matters more now that connection authorization is enforced: a 403 would read as
"nothing captured yet" and send the user to re-run an ingestion they cannot fix. Added
a shared QueryError component wired into CustomerExplorer (3 branches), QueryTrendsTab
(2) and WorkloadAnalysisPanel (1), with per-status wording verified against the real
interceptor error shapes.

Tab bar and ARIA

At 390 px the four tabs measured 427 px with overflow-x: visible, so three of them sat
off-screen unreachable. The bar now scrolls (verified: scrolls 373 px, all four
reachable). Completed the ARIA tabs pattern — role="tabpanel", aria-controls, roving
tabindex and arrow/Home/End navigation. The first version had a stale-closure bug that
moved selection exactly once and then froze; fixed with the functional state updater,
verified across the full key sequence including wraparound.

Workload reads were gated on the write tier

status/latest/getReport/history all used assertCanManageConnectionContent.
EffectiveConnectionAccess's own comment lists slow-query analytics under read. Latent
today because every grant resolves to FULL_CONTENT, but it would deny the whole
Workload tab to a read-only grant the moment one is reintroduced. `run` keeps manage.

Known issue, documented rather than fixed

The slow-log ingestion cursor has two real defects, both left in place with a comment
at updateLastProcessed explaining them: it records the time ingestion *finished*
rather than the last event's timestamp (so events arriving mid-run are skipped
permanently), and it writes LocalDateTime.now() while every read does
.atZone(ZoneOffset.UTC) (agreeing only because the container runs Etc/UTC; a
bare-metal install at UTC+5:30 would skip 5.5 h of history every run). Not fixed here
because all six providers need live cloud credentials to exercise, and an unverified
change to a cursor silently skips or duplicates data.

Verification

Backend and frontend both build clean. Live against the rebuilt image: new customer
routes 200 with a slash-bearing id and 403 on an ungranted connection; workload reads
non-403 for a granted user; the lineage index confirmed as an Index Scan on the real
table.

Not covered: mvn test was not run locally (no JDK/Maven on this host) — CI runs it.
Note that the "backend tests (advisory)" job is continue-on-error, so its green tick
means the job finished, not that tests passed; main currently has 5 failing test
classes independent of this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Matching

Found in hands-on QA of the previous commit, not by reading it.

SlowQueryAnalyticsService.normalizeForMatching ends with .trim(); the SQL
expression behind the generated column did not. A lineage row stored with
leading whitespace therefore normalized to " select ..." in the column and
"select ..." on the Java side, so the prefix LIKE never matched and
recoverFullText silently returned the truncated sample instead of the full
SQL. Verified against the local install: inserting '   SELECT x FROM t
WHERE y = 1   ' produced "[ select x from t where y = 1 ]" where Java
produces "[select x from t where y = 1]"; 4 of 1,174 real rows carried such
whitespace.

The flaw was equally present in the inline expression this column replaced,
so it is pre-existing rather than a regression — but it is silent either
way, which is why it survived.

Two parts to the fix:

* btrim(...) added to the expression in both the initializer and V118.

* The initializer now detects a stale column and rebuilds it. A generated
  column's expression cannot be altered in place and ADD COLUMN IF NOT
  EXISTS silently keeps whatever is already there, so an install that ran
  the earlier build would have kept the untrimmed expression forever. It
  compares pg_get_expr against the expected shape and only drops/re-adds
  when they differ, so a normal restart does not rewrite the table.

Verified on the running stack: restart logged "Rebuilding
query_lineage.normalized_match: stored expression is out of date", the
stored expression now carries btrim, the index survived the rebuild, and
rows-with-untrimmed-normalization went from 4 to 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@notSumit25
notSumit25 requested a review from a team as a code owner August 29, 2026 12:17
@notSumit25

Copy link
Copy Markdown
Collaborator Author

What's done

The seven non-blocking findings left over from the Performance-tab readiness review after #86. Each was re-verified against current main before being fixed, and each fix verified after — on the running stack, not by reading the diff.

# Gap Fix Evidence
1 Sample recovery was an unindexable full scan STORED generated column + text_pattern_ops index Seq Scan 37.95 ms → Index Scan 0.062 ms on the same data
2 query_lineage never pruned Added to SlowQueryRetentionService planted 400d/90d/2d rows → deleted exactly the 2 old ones
3 retry: 3 retried denials Status predicate on the query client 4 attempts / 7197 ms → 1 / 34 ms
4 Slash in customerId unreachable Moved the id off the path to a query param acct/77?x=1 → 200 (old route: 404 raw, 400 encoded)
5 Failed fetch rendered as "no data yet" Shared QueryError across 6 branches real 403 now renders an alert with the denial text
6 3 of 4 tabs off-screen at 390px Scrolling tab bar scrolls 263px; 4th tab actually clicked at mobile width
7 Workload reads gated on the write tier assertCanReadConnectionContent 4 reads / 1 write; reads non-403 for a granted user

Three things worth calling out

Two of my own fixes were wrong on the first attempt, and only testing caught them:

  • The retry fix did nothing. I read error.response.status, but the axios interceptor rethrows a plain Error with the status on error.status — the predicate never matched and the denied request still made 4 attempts over 7 seconds. Measured in the browser, not inferred.
  • The arrow-key handler had a stale closure: selection moved exactly once, then froze. Fixed with the functional state updater.

I nearly shipped a COALESCE fallback on the lineage query "to be safe on older Postgres". Tested it: COALESCE(normalized_match, …) plans a Seq Scan at 48.7 ms vs 0.39 ms for the bare column — it would have silently negated the whole optimization. Removed, with the measurement recorded in the code so nobody re-adds it.

Hands-on QA found a real silent bug (second commit, 06476bf) that reading the code would not have surfaced. Java's normalizeForMatching ends with .trim(); the SQL expression behind the generated column did not. A row stored with leading whitespace normalized to " select ..." in the column and "select ..." in Java, so the prefix LIKE never matched and recovery silently returned the truncated sample. 4 of 1,174 real rows carried such whitespace. The same flaw was in the inline expression this replaces, so it is pre-existing rather than a regression — but silent either way, which is why it survived.

That fix has two parts: btrim(...) in the expression, and stale-column detection in the initializer. A generated column's expression cannot be altered in place and ADD COLUMN IF NOT EXISTS silently keeps whatever is already there, so any install that ran the earlier build would have kept the untrimmed expression forever. It compares pg_get_expr and only drops/re-adds when they differ, so a normal restart doesn't rewrite the table. Verified: restart logged Rebuilding query_lineage.normalized_match: stored expression is out of date, the index survived, affected rows went 4 → 0.

Deliberately not fixed

The slow-log ingestion cursor has two real defects, documented at updateLastProcessed rather than changed:

  1. It records when ingestion finished, not the last event's timestamp — so events arriving mid-run are skipped permanently, a gap proportional to run duration.
  2. It writes LocalDateTime.now() while every read does .atZone(ZoneOffset.UTC). These agree only because the container runs Etc/UTC; a bare-metal install at UTC+5:30 would skip 5.5 h of history every run (west of UTC it re-ingests duplicates).

All six providers need live cloud credentials to exercise, and an unverified change to an ingestion cursor silently skips or duplicates production data. Better documented than guessed at.

Verification

Backend and frontend both build clean. 21 hands-on QA scenarios run against the local stack — API + direct DB query + real browser via Chrome DevTools MCP, with API/DB/UI agreement required for a pass. Highlights: the slash-id round trip renders SELECT * FROM orders WHERE customer_id = 'acct/77?x=1' in the samples modal; a genuine mid-session grant revocation makes the panel show "You don't have access to this connection's queries. (HTTP 403)" as an assertive alert; 12 adjacent endpoints still 200 (no regressions). All test data cleaned up and both password hashes restored byte-identical.

On the index: at the current 799 rows the planner correctly prefers a seq scan, and idx_scan stays 0 — that is right, not a defect. At 36,456 rows it chooses the Index Scan unprompted (0.038 ms), which is the case the fix targets.

Caveat on CI

mvn test was not run locally (no JDK/Maven on this host). Note that backend tests (advisory) is continue-on-error: true, so a green tick there means the job finished, not that tests passed — main currently has 5 failing test classes independent of this branch. Worth reading the actual test output rather than the check mark.

@notSumit25 notSumit25 added the product: query-performance Slow query analysis, fingerprinting, ranking and baseline regressions label Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product: query-performance Slow query analysis, fingerprinting, ranking and baseline regressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant